-
Notifications
You must be signed in to change notification settings - Fork 999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PeerDAS: Add KZG verification when sampling #14187
Conversation
if !verified { | ||
return pubsub.ValidationReject, errors.New("failed to verify kzg proof of column") | ||
} | ||
|
||
// ????? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nisdas to what specification rule this portion of code does correspond?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are just retrieving the state here, it doesnt correspond to any specification rule directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I was more talking about the following block:
if err := coreBlocks.VerifyBlockHeaderSignatureUsingCurrentFork(parentState, ds.SignedBlockHeader); err != nil {
return pubsub.ValidationReject, err
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[REJECT] The proposer signature of sidecar.signed_block_header, is valid with respect to the block_header.proposer_index pubkey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 6431659
875ac52
to
3a36a70
Compare
3a36a70
to
330a87e
Compare
if !verified { | ||
return pubsub.ValidationReject, errors.New("failed to verify kzg proof of column") | ||
} | ||
|
||
// ????? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are just retrieving the state here, it doesnt correspond to any specification rule directly
if err != nil { | ||
return pubsub.ValidationIgnore, err | ||
// Add specific debug log. | ||
if logrus.GetLevel() >= logrus.DebugLevel { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think there is any reason to gate it this way as a time calculation is very cheap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in b4ad914.
* `validateDataColumn`: Add comments and remove debug computation. * `sampleDataColumnsFromPeer`: Add KZG verification * `VerifyKZGInclusionProofColumn`: Add unit test. * Make deepsource happy. * Address Nishant's comment. * Address Nishant's comment.
* `validateDataColumn`: Add comments and remove debug computation. * `sampleDataColumnsFromPeer`: Add KZG verification * `VerifyKZGInclusionProofColumn`: Add unit test. * Make deepsource happy. * Address Nishant's comment. * Address Nishant's comment.
* `validateDataColumn`: Add comments and remove debug computation. * `sampleDataColumnsFromPeer`: Add KZG verification * `VerifyKZGInclusionProofColumn`: Add unit test. * Make deepsource happy. * Address Nishant's comment. * Address Nishant's comment.
* `validateDataColumn`: Add comments and remove debug computation. * `sampleDataColumnsFromPeer`: Add KZG verification * `VerifyKZGInclusionProofColumn`: Add unit test. * Make deepsource happy. * Address Nishant's comment. * Address Nishant's comment.
* `validateDataColumn`: Add comments and remove debug computation. * `sampleDataColumnsFromPeer`: Add KZG verification * `VerifyKZGInclusionProofColumn`: Add unit test. * Make deepsource happy. * Address Nishant's comment. * Address Nishant's comment.
* `validateDataColumn`: Add comments and remove debug computation. * `sampleDataColumnsFromPeer`: Add KZG verification * `VerifyKZGInclusionProofColumn`: Add unit test. * Make deepsource happy. * Address Nishant's comment. * Address Nishant's comment.
* `validateDataColumn`: Add comments and remove debug computation. * `sampleDataColumnsFromPeer`: Add KZG verification * `VerifyKZGInclusionProofColumn`: Add unit test. * Make deepsource happy. * Address Nishant's comment. * Address Nishant's comment.
* `validateDataColumn`: Add comments and remove debug computation. * `sampleDataColumnsFromPeer`: Add KZG verification * `VerifyKZGInclusionProofColumn`: Add unit test. * Make deepsource happy. * Address Nishant's comment. * Address Nishant's comment.
* `validateDataColumn`: Add comments and remove debug computation. * `sampleDataColumnsFromPeer`: Add KZG verification * `VerifyKZGInclusionProofColumn`: Add unit test. * Make deepsource happy. * Address Nishant's comment. * Address Nishant's comment.
* `validateDataColumn`: Add comments and remove debug computation. * `sampleDataColumnsFromPeer`: Add KZG verification * `VerifyKZGInclusionProofColumn`: Add unit test. * Make deepsource happy. * Address Nishant's comment. * Address Nishant's comment.
* `validateDataColumn`: Add comments and remove debug computation. * `sampleDataColumnsFromPeer`: Add KZG verification * `VerifyKZGInclusionProofColumn`: Add unit test. * Make deepsource happy. * Address Nishant's comment. * Address Nishant's comment.
* `validateDataColumn`: Add comments and remove debug computation. * `sampleDataColumnsFromPeer`: Add KZG verification * `VerifyKZGInclusionProofColumn`: Add unit test. * Make deepsource happy. * Address Nishant's comment. * Address Nishant's comment.
* `validateDataColumn`: Add comments and remove debug computation. * `sampleDataColumnsFromPeer`: Add KZG verification * `VerifyKZGInclusionProofColumn`: Add unit test. * Make deepsource happy. * Address Nishant's comment. * Address Nishant's comment.
* `validateDataColumn`: Add comments and remove debug computation. * `sampleDataColumnsFromPeer`: Add KZG verification * `VerifyKZGInclusionProofColumn`: Add unit test. * Make deepsource happy. * Address Nishant's comment. * Address Nishant's comment.
* `validateDataColumn`: Add comments and remove debug computation. * `sampleDataColumnsFromPeer`: Add KZG verification * `VerifyKZGInclusionProofColumn`: Add unit test. * Make deepsource happy. * Address Nishant's comment. * Address Nishant's comment.
* `validateDataColumn`: Add comments and remove debug computation. * `sampleDataColumnsFromPeer`: Add KZG verification * `VerifyKZGInclusionProofColumn`: Add unit test. * Make deepsource happy. * Address Nishant's comment. * Address Nishant's comment.
* `validateDataColumn`: Add comments and remove debug computation. * `sampleDataColumnsFromPeer`: Add KZG verification * `VerifyKZGInclusionProofColumn`: Add unit test. * Make deepsource happy. * Address Nishant's comment. * Address Nishant's comment.
* `validateDataColumn`: Add comments and remove debug computation. * `sampleDataColumnsFromPeer`: Add KZG verification * `VerifyKZGInclusionProofColumn`: Add unit test. * Make deepsource happy. * Address Nishant's comment. * Address Nishant's comment.
Please read commit by commit.
Before this PR,
VerifyKZGInclusionProofColumn
andVerifyDataColumnSidecarKZGProofs
were not evaluated when receiving columns from a peer during sampling.Previously, only the root and the column index were evaluated. This allowed a peer to submit a fabricated column with a correct root and index, which posed a security risk.
Now,
VerifyKZGInclusionProofColumn
andVerifyDataColumnSidecarKZGProofs
are evaluated for every sampled column, and the corresponding columns are rejected if these evaluations fail.